-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[misc] acceptance test GHA workflow #897
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #897 +/- ##
=======================================
Coverage 86.81% 86.81%
=======================================
Files 72 72
Lines 5255 5255
=======================================
Hits 4562 4562
Misses 693 693
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -1,5 +1,5 @@ | |||
[build-system] | |||
requires = ["setuptools>=45", "setuptools_scm[toml]>=6.2"] | |||
requires = ["setuptools>=64", "setuptools_scm[toml]>=8"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only tangentially related to the new GHA, but important as the existing dependencies were quite obsolete and willl fail in some scenarios. See https://setuptools-scm.readthedocs.io/en/latest/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, w/some very minor suggestions.
|
||
r_unit_tests: | ||
if: false # TEMPORARILY DISABLE FOR DEBUGGING | ||
runs-on: single-cell-1tb-runner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woot! just wanted to say 👋🏻 and introduce myself. I worked with @beroy getting these self-hosted runners set up. I'm watching them periodically and recording metrics on the nodes and autoscaler, but ping me if you see any unusual behavior or have other requirements you want us to add to them. Have fun!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @jakeyheath - these are a game changer. Really happy to have them!
So far they seem to work great, other than missing some "nice-to-have" packages installed in their default image. I have given @beroy an initial list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easy to solve, we already chatted this morning about building a custom container for this project to use that has all the packages pre-installed.
Let's meet in the new year and see how things are going. Right now, everything is running in our czi-playground account. Might be good to put this project is a more specific location and tune the performance as needed. Here's a screenshot of the metrics I collected on the latest run. Happy to show yall how to pull these yourselves and tune to your needs.
@@ -209,7 +209,7 @@ def test_hvg_vs_scanpy( | |||
"Homo sapiens", | |||
"is_primary_data == True", | |||
"dataset_id", | |||
slice(500_000, 1_000_000), | |||
slice(1_000_000, 4_000_000), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the latest LTS census has no primary cells in the [500_000,1_000_000] soma joniid range, so this test will fail. Expanded the range to ensure we pick up some cells.
@@ -102,7 +102,7 @@ test_that("test_incremental_read_X_human", { | |||
}) | |||
|
|||
test_that("test_incremental_read_X_human-large-buffer-size", { | |||
census <- open_soma_latest_for_test(soma.init_buffer_bytes = paste(4 * 1024**3)) | |||
census <- open_soma_latest_for_test(soma.init_buffer_bytes = paste(1 * 1024**3)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on very high cpu count machines, this will OOM due to per-core buffer allocation. Reducing to a GiB resolves the issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Let's ship those now and monitor how they go.
Add GHA which peforms the R and Python "acceptance" tests on a schedule - currently every Saturday at 1AM UTC. Can also be run manually via workflow_dispatch.
By default, will install from
main
and run the latest acceptance tests inmain
. You can run it against a branch with:The python (not R) job supports installing a custom
tiledbsoma
version, allowing themain
of this repo to test against any tiledbsoma version, including branches from that repo. This will allow pre-qualification of tiledbsoma releases against the latest cellxgene-census code base. For example, to test against the head of main, do:Other changes:
Current test run status:
Overhang:
libtiledbsoma.so
intopackage_data
if it already exists single-cell-data/TileDB-SOMA#1937 contains a fix that will allow removal of theeditable
flag on the tiledbsoma version override when fixed. Filed as remove editable flag from full-unittest.yml workflow #907